feat: Add client-side language server configuration with per-language case sensitivity as the first available option (issue 626) #659

SCWells72 · 2024-12-02T21:04:11Z

This should be a pretty complete implementation of client-side configuration via JSON based on prior discussions with @angelozerr. As expected, the first client-side configuration option is a language-level option designating the case-sensitivity of the associated grammar. That option defaults to false for backward-compatibility, and I've added default client-side settings to the all existing language server templates with the appopriate value for caseSensitive. Unsurprisingly, only one template-supported language, HTML, is case-insensitive.

…language ID settings. The only client-side setting right now is whether or not the language's grammar is case-sensitive (defaults to false for backward-compatibility), and that setting is used when creating completion lookup elements with the corresponding case-sensitivity. I've added "clientSettings.json" defaults for the TypeScript and CSS language servers and can stub the others as appropriate.

…ent-side configuration.

SCWells72 · 2024-12-02T21:05:36Z

src/main/java/com/redhat/devtools/lsp4ij/LanguageServersRegistry.java

@@ -162,7 +162,8 @@ private void loadServersAndMappingFromSettings() {
                                launch.getUserEnvironmentVariables(),
                                launch.isIncludeSystemEnvironmentVariables(),
                                launch.getConfigurationContent(),
-                                launch.getInitializationOptionsContent()),
+                                launch.getInitializationOptionsContent(),
+                                launch.getClientConfigurationContent()),


There's obviously going to be a proliferation of these types of changes where I've added clientConfiguration as another property of UserDefinedLanguageServerDefinition and its peer types/methods/etc.

SCWells72 · 2024-12-02T21:06:17Z

src/main/java/com/redhat/devtools/lsp4ij/LanguageServersRegistry.java

@@ -483,13 +486,15 @@ private void removeAssociationsFor(LanguageServerDefinition definition) {
        boolean mappingsChanged = !Objects.deepEquals(settings.getMappings(), request.mappings());
        boolean configurationContentChanged = !Objects.equals(settings.getConfigurationContent(), request.configurationContent());
        boolean initializationOptionsContentChanged = !Objects.equals(settings.getInitializationOptionsContent(), request.initializationOptionsContent());
+        // Not checking whether client config changed because that shouldn't result in a LanguageServerChangedEvent


Specifically calling out that we don't need to include client config changes when computing whether or not to send an event here because the actual language server doesn't care at all about client-side config changes.

For the moment I agree with you. Lets keep like this, but we could have a client settings which could impact the editor (like enable/disable lsp codelens which should refresh the editor and in this case we should send an event)

SCWells72 · 2024-12-02T21:07:34Z

src/main/java/com/redhat/devtools/lsp4ij/client/features/LSPCompletionFeature.java

-                    .caseInsensitive()
-                    .addElement(prioritizedLookupItem);
+            // Add the IJ completion item (lookup item) by using the computed prefix respecting the language's case-sensitivity
+            if (caseSensitive) {


I could condense these two conditionals slightly be extracting result.withPrefixMatcher() into a variable and setting other state conditionally, but I didn't find that quite as readable.

I need to revisit this addLookupItem method so please dont loose your time about it

SCWells72 · 2024-12-02T21:08:30Z

src/main/java/com/redhat/devtools/lsp4ij/features/completion/LSPCompletionContributor.java

            }
        }
    }

+    private static boolean isCaseSensitive(@NotNull LanguageServerItem languageServer, @NotNull PsiFile file) {


This should be pretty straightforward, but basically try to infer language grammar case-sensitivity via the file => language ID => client-side settings.

SCWells72 · 2024-12-02T21:09:02Z

src/main/java/com/redhat/devtools/lsp4ij/launching/UserDefinedLanguageServerSettings.java

@@ -119,6 +119,8 @@ public static class UserDefinedLanguageServerItemSettings {

        private String initializationOptionsContent;

+        private String clientConfigurationContent = "{}";


The default value of {} here is what allows smooth backward-compatibility with existing language server definitions. I think there is an open question of whether we should try to update existing language server definitions for case-sensitive languages with a client config saying as much. I don't know if there's any kind of existing precedent for updating existing definitions with new config information...

SCWells72 · 2024-12-02T21:10:36Z

src/main/java/com/redhat/devtools/lsp4ij/launching/templates/LanguageServerTemplate.java

@@ -40,6 +40,7 @@ public String getName() {
    public static final String TEMPLATE_FILE_NAME = "template.json";
    public static final String INITIALIZATION_OPTIONS_FILE_NAME = "initializationOptions.json";
    public static final String SETTINGS_FILE_NAME = "settings.json";
+    public static final String CLIENT_SETTINGS_FILE_NAME = "clientSettings.json";


Following the existing pattern of the server-side config file being named settings.json and naming the client-side config clientSettings.json.

SCWells72 · 2024-12-02T21:11:20Z

src/main/java/com/redhat/devtools/lsp4ij/launching/templates/LanguageServerTemplateManager.java

@@ -17,25 +17,26 @@
 import com.intellij.openapi.vfs.JarFileSystem;
 import com.intellij.openapi.vfs.VfsUtilCore;
 import com.intellij.openapi.vfs.VirtualFile;
+import com.redhat.devtools.lsp4ij.internal.StringUtils;


Many of the changes to this file are from just running the Java formatter using the project code style.

SCWells72 · 2024-12-02T21:11:48Z

src/main/java/com/redhat/devtools/lsp4ij/launching/templates/LanguageServerTemplateManager.java

@@ -131,6 +135,7 @@ public LanguageServerTemplate createLsTemplate(@NotNull VirtualFile templateFold
        String templateJson = null;
        String settingsJson = null;
        String initializationOptionsJson = null;
+        String clientSettingsJson = null;


The real changes in this file begin here where we add handling for clientSettings.json.

SCWells72 · 2024-12-02T21:12:59Z

.../redhat/devtools/lsp4ij/server/definition/launching/ClientConfigurationLanguageSettings.java

+/**
+ * Language-specific client-side settings for a user-defined language server configuration.
+ */
+public class ClientConfigurationLanguageSettings {


These are the actual per-language client-side settings. Again, there's only one right now, caseSensitive, but it I think we've discussed a few others in the context of other issues/PRs.

SCWells72 · 2024-12-02T21:13:51Z

.../redhat/devtools/lsp4ij/server/definition/launching/UserDefinedLanguageServerDefinition.java

+    public Map<String, ClientConfigurationLanguageSettings> getLanguageServerClientConfiguration() {
+        if ((clientConfiguration == null) && (clientConfigurationContent != null) && !clientConfigurationContent.isBlank()) {
+            try {
+                clientConfiguration = JSONUtils.getLsp4jGson().fromJson(clientConfigurationContent, ClientConfigurationLanguageSettings.TYPE_TOKEN);


Unlike the server-side settings and initialization options, we have a concrete schema here and can deserialize into a specific type, in this case, a map keyed by language ID with values that are per-language client-side settings.

SCWells72 · 2024-12-02T21:15:18Z

...n/java/com/redhat/devtools/lsp4ij/settings/jsonSchema/AbstractLSPJsonSchemaFileProvider.java

+/**
+ * Abstract base class for JSON schema file providers that are based on JSON schema files bundled in the plugin distribution.
+ */
+abstract class AbstractLSPJsonSchemaFileProvider implements JsonSchemaFileProvider {


Now we can get back to JSON schema-based validation given that we have a well-defined schema for our client-side configuration. This is the abstract base class for the one I've added and any others that might be needed in the future. It already encapsulates loading the JSON schema definition from the plugin distribution.

SCWells72 · 2024-12-02T21:15:43Z

...redhat/devtools/lsp4ij/settings/jsonSchema/LSPClientConfigurationJsonSchemaFileProvider.java

+/**
+ * JSON schema file provider for language server client-side configuration.
+ */
+public class LSPClientConfigurationJsonSchemaFileProvider extends AbstractLSPJsonSchemaFileProvider {


And this is the first concrete JSON schema file provider for client-side config.

SCWells72 · 2024-12-02T21:16:04Z

src/main/java/com/redhat/devtools/lsp4ij/settings/jsonSchema/LSPJsonSchemaProviderFactory.java

+/**
+ * Factory for JSON schema file providers that are based on JSON schema files bundled in the plugin distribution.
+ */
+public class LSPJsonSchemaProviderFactory implements JsonSchemaProviderFactory {


And the EP by which the JSON schema file providers are actually registered.

SCWells72 · 2024-12-02T21:16:46Z

src/main/java/com/redhat/devtools/lsp4ij/settings/ui/JsonTextField.java

+     *
+     * @param jsonFilename the JSON file name
+     */
+    public void setJsonFilename(@NotNull String jsonFilename) {


This allows us to make a JsonTextField use one of the registered JSON schema file providers based on the associated file name.

SCWells72 · 2024-12-02T21:17:42Z

src/main/java/com/redhat/devtools/lsp4ij/settings/ui/LanguageServerPanel.java

-        createConfigurationField(configurationTab);
-        createInitializationOptionsTabField(configurationTab);
+
+        JBTabbedPane configurationTabbedPane = new JBTabbedPane();


Made the Configuration tab support sub-tabs for server-side and client-side configuration. Server-side configuration is just what was there before immediately under Configuration.

SCWells72 · 2024-12-02T21:18:13Z

src/main/java/com/redhat/devtools/lsp4ij/settings/ui/LanguageServerPanel.java

@@ -250,15 +260,21 @@ private static JLabel createLabelForComponent(@NotNull @NlsContexts.Label String
    }

    private void createConfigurationField(FormBuilder builder) {
-        configurationWidget = new LanguageServerConfigurationWidget(project);
+        configurationWidget = new JsonTextField(project);


I removed the explcit subclasses of JsonTextField as they weren't adding any real value.

SCWells72 · 2024-12-02T21:19:38Z

src/main/resources/META-INF/plugin.xml

@@ -158,6 +158,7 @@
    <depends>com.intellij.modules.platform</depends>
    <depends optional="true">org.jetbrains.plugins.textmate</depends>
    <depends optional="true">com.redhat.devtools.intellij.telemetry</depends>
+    <depends optional="true" config-file="plugin-json.xml">com.intellij.modules.json</depends>


Added the optional dependency for JSON schema validation using the proper mechanism of loading the associated plugin EP implementations via the separate file above. It would likely make sense to do the same with the other two optional dependencies if possible.

SCWells72 · 2024-12-02T21:19:56Z

src/main/resources/jsonSchema/clientSettings.schema.json

@@ -0,0 +1,20 @@
+{


JSON schema for client-side configuration.

SCWells72 · 2024-12-02T21:20:27Z

src/main/resources/messages/LanguageServerBundle.properties

 language.server.configuration=Configuration:
-language.server.configuration.emptyText=Defines the configuration of the language server


These two messages were unused after the move to JsonTextField which doesn't support placeholder text.

SCWells72 · 2024-12-02T21:20:49Z

src/main/resources/messages/LanguageServerBundle.properties

@@ -33,10 +33,10 @@ language.server.mappings.fileNamePattern.no=No file name patterns mappings confi
 language.server.mappings.languageId.column=Language Id

 language.server.tab.configuration=Configuration
+language.server.tab.configuration.server=Server configuration


These are the configuration sub-tab labels. Let me know if you'd prefer something else.

SCWells72 · 2024-12-02T21:21:21Z

src/main/resources/templates/typescript-language-server/clientSettings.json

@@ -0,0 +1,8 @@
+{


Client-side config defaults for the TypeScript language server template.

SCWells72 · 2024-12-02T21:21:27Z

src/main/resources/templates/vscode-css-language-server/clientSettings.json

@@ -0,0 +1,5 @@
+{


Client-side config defaults for the CSS language server template.

…s not surprisingly, all languages but one -- HTML -- are case-sensitive. That makes me wonder if perhaps the default should be case-sensitive and HTML the exception to the rule. I'm also not sure if the language IDs for lemminx are correct as they're not specified explicitly in template.json. I've just used the language names in lower-case.

SCWells72 · 2024-12-02T22:04:46Z

src/main/resources/templates/lemminx/clientSettings.json

@@ -0,0 +1,11 @@
+{
+  "xml": {


I don't know if the language IDs are correct here as they're not explicitly speicfied in this template's template.json. All three languages are case-sensitive, though.

SCWells72 · 2024-12-02T22:47:52Z

src/main/resources/jsonSchema/clientSettings.schema.json

+  "type": "object",
+  "additionalProperties": false,
+  "patternProperties": {
+    "^[a-zA-Z0-9-_]+$": {


Let me know if you're aware of language IDs that wouldn't be matched by this.

angelozerr · 2024-12-03T10:14:14Z

src/main/java/com/redhat/devtools/lsp4ij/features/completion/LSPCompletionContributor.java

@@ -125,6 +129,9 @@ private void addCompletionItems(@NotNull CompletionParameters parameters,
        items.sort(completionProposalComparator);
        int size = items.size();

+        // Determine whether or not the language is case-sensitive so that we can add completions accordingly
+        boolean caseSensitive = isCaseSensitive(languageServer, parameters.getOriginalFile());


To be consistent with IJ, I would rename with isCaseInsensitive

angelozerr · 2024-12-03T10:21:44Z

src/main/java/com/redhat/devtools/lsp4ij/client/features/LSPCompletionFeature.java

     */
    @ApiStatus.Internal
    public void addLookupItem(@NotNull CompletionPrefix completionPrefix,
                              @NotNull CompletionResultSet result,
                              @NotNull LookupElement lookupItem,
                              int priority,
-                              @NotNull CompletionItem item) {
+                              @NotNull CompletionItem item,
+                              boolean caseSensitive) {


Even if it is internal, please try to avoid updating the signature. The caseSensitive can be get inside the method.

Disregard the previous comment. Once I migrated stuff into client features it all made more sense!

You don't need server definition, you must work with LSPCompeltionfeatures#caseIsensitive(PsiFile file)

The idea is that UserDefinedLanguageServerDefinition will return a custom class UserDefinedCompletionFeatures which impelemnts caseIsensitive(PsiFile file)

angelozerr · 2024-12-03T10:30:15Z

src/main/java/com/redhat/devtools/lsp4ij/features/completion/LSPCompletionContributor.java

+    private static boolean isCaseSensitive(@NotNull LanguageServerItem languageServer, @NotNull PsiFile file) {
+        LanguageServerDefinition serverDefinition = languageServer.getServerDefinition();
+        if (serverDefinition instanceof UserDefinedLanguageServerDefinition languageServerDefinition) {
+            Map<String, ClientConfigurationLanguageSettings> clientConfiguration = languageServerDefinition.getLanguageServerClientConfiguration();


This code must not be here but in the custom implementation for user defined of ClientFeatures which will register a custom CompletionFeatures and implement the new method isCaseInsensitive(PsiFile)

…DefinedCompletionFeature.

…nsitive() should live.

…ed in the user-defined portions.

SCWells72 · 2024-12-03T17:31:06Z

src/main/java/com/redhat/devtools/lsp4ij/client/features/LSPCompletionFeature.java

                              @NotNull CompletionResultSet result,
                              @NotNull LookupElement lookupItem,
                              int priority,
                              @NotNull CompletionItem item) {
+        // Determine whether or not completions should be case-sensitive
+        boolean caseSensitive = isCaseSensitive(context.getParameters().getOriginalFile());


Note that both getParameters() and getOriginalFile() are annotated as @NotNull, so this is safe.

SCWells72 · 2024-12-03T17:33:49Z

src/main/resources/templates/clojure-lsp/clientSettings.json

@@ -0,0 +1,5 @@
+{


All of the languages integrated via these templates are for case-sensitive languages except for HTML (see below).

SCWells72 · 2024-12-03T17:34:05Z

src/main/resources/templates/vscode-html-language-server/clientSettings.json

@@ -0,0 +1,5 @@
+{
+  "completions": {
+    "caseSensitive": false


This is the only one that's case-insensitive.

angelozerr · 2024-12-03T21:14:32Z

...ava/com/redhat/devtools/lsp4ij/server/definition/launching/UserDefinedCompletionFeature.java

+public class UserDefinedCompletionFeature extends LSPCompletionFeature {
+    public boolean isCaseSensitive(@NotNull PsiFile file) {
+        LanguageServerDefinition serverDefinition = getClientFeatures().getServerDefinition();
+        if (serverDefinition instanceof UserDefinedLanguageServerDefinition languageServerDefinition) {


This condition is always true

I'm fine changing it to an immediate unchecked cast if you are, but getServerDefinition() in that expression returns LanguageServerDefinition and not UserDefinedLanguageServerDefinition, hence the instanceof check before safely casting. What's your preference?

Cast it directly since we are sure that it is a user defined ls

Done. Both of those feedback items are implemented. Let me know when you've completed your feedback and, once I've addressed all of it, I'll push another commit.

angelozerr · 2024-12-03T21:19:12Z

...ava/com/redhat/devtools/lsp4ij/server/definition/launching/UserDefinedCompletionFeature.java

+            return (clientConfiguration != null) && clientConfiguration.completions.caseSensitive;
+        }
+        // Default to case-insensitive if unspecified for backward-compatibility
+        return false;


I will return the super.isCaseSensitive

angelozerr · 2024-12-04T03:43:58Z

I have tested the PR and it works very good, amazing job @SCWells72 !

There are few changes to do, but I merge the PR and I will create a little PR to fix few code.

Thanks @SCWells72 !

SCWells72 added 2 commits December 2, 2024 14:35

Issue 626 - Added JSON schema-based completion and validation for cli…

cbb923f

…ent-side configuration.

SCWells72 commented Dec 2, 2024

View reviewed changes

src/main/resources/jsonSchema/clientSettings.schema.json

@@ -0,0 +1,20 @@

{

Copy link

Contributor Author

SCWells72 Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JSON schema for client-side configuration.

SCWells72 commented Dec 2, 2024

View reviewed changes

SCWells72 mentioned this pull request Dec 2, 2024

Code completion should be case-sensitive if the language is case-sensitive #626

Closed

SCWells72 commented Dec 2, 2024

View reviewed changes

Issue 626 - Made the JSON schema for client settings more strict.

e898618

SCWells72 commented Dec 2, 2024

View reviewed changes

angelozerr reviewed Dec 3, 2024

View reviewed changes

SCWells72 added 3 commits December 3, 2024 10:22

Issue 626 - Implemented almost all feedback. Still need to add a User…

500d7fd

…DefinedCompletionFeature.

Issue 626 - Added a TODO for the remaining work around where isCaseSe…

bc4d8ef

…nsitive() should live.

Issue 626 - Refactored the new case-sensitivity stuff to be implement…

696165d

…ed in the user-defined portions.

SCWells72 commented Dec 3, 2024

View reviewed changes

angelozerr reviewed Dec 3, 2024

View reviewed changes

angelozerr merged commit 35dc626 into redhat-developer:main Dec 4, 2024
6 checks passed

SCWells72 deleted the issue_626 branch December 4, 2024 14:29

		@@ -119,6 +119,8 @@ public static class UserDefinedLanguageServerItemSettings {

		private String initializationOptionsContent;

		private String clientConfigurationContent = "{}";

		language.server.configuration=Configuration:
		language.server.configuration.emptyText=Defines the configuration of the language server

feat: Add client-side language server configuration with per-language case sensitivity as the first available option (issue 626) #659

feat: Add client-side language server configuration with per-language case sensitivity as the first available option (issue 626) #659

Conversation

SCWells72 commented Dec 2, 2024 • edited Loading

SCWells72 Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SCWells72 Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SCWells72 Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SCWells72 Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angelozerr Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angelozerr commented Dec 4, 2024

SCWells72 commented Dec 2, 2024 •

edited

Loading

SCWells72 Dec 2, 2024 •

edited

Loading

SCWells72 Dec 2, 2024 •

edited

Loading

SCWells72 Dec 2, 2024 •

edited

Loading

SCWells72 Dec 3, 2024 •

edited

Loading

angelozerr Dec 3, 2024 •

edited

Loading